Populate package-path metadata into package context #3596

justinsb · 2022-10-03T14:17:36Z

We construct a path based on the hierarchy of parent packages. This
is useful for packages/package functions that want to construct values
meaningful beyond just their package - for example domain names, GCP
project IDs, bucket names, descriptions etc.

droot

drive by questions.

droot · 2022-10-03T18:51:56Z

porch/pkg/engine/engine.go

@@ -104,10 +112,56 @@ func (cad *cadEngine) ListPackageRevisions(ctx context.Context, repositorySpec *
 	return repo.ListPackageRevisions(ctx, filter)
 }

-func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, obj *api.PackageRevision) (repository.PackageRevision, error) {
+func buildPackageConfig(ctx context.Context, obj *api.PackageRevision, parent repository.PackageRevision) (*builtins.PackageConfig, error) {


Can you remind me.. what parent means in context of porch ? few things popping up in my mind: upstream package, parent of a draft package and parent package of a subpackage (which we don't support in porch yet).

Not sure I've got the best name, but parent will let us establish a "position" of the package, in the same way that the directory structure does in git.

This proves to be a useful concept, I think, for value propagation and just to organize things.

This isn't done yet, but I would hope we could map this to directories in a git repo, so we would likely need this for CLI/server parity.

positionalParent is how I will remember this now :)

droot · 2022-10-03T18:53:22Z

internal/builtins/pkg_context.go

+type PackageConfig struct {
+	// PackagePath is the path to the package, as determined by the names of the parent packages.
+	// The path to a package is the parent package path joined with the package name.
+	PackagePath string


Can you share what this is going to look like in CLI and porch based workflow. Is it relative ?

So as I have it now, it's going to look like a directory path, myorg/apps/coolapp/dev or something. I'm not certain what comes at the start of that (for example, should we try to make these globally unique, or at least allow that, vs being relative to the repository?) I'm also not certain whether we should include some notion of the package type, in which case the package path would end up looking like a GCP URL organizations/myorg/folders/apps/applications/coolapp/environments/dev.

I figure we can iterate a bit here.

I think it would be the same in the CLI, though I'm not yet sure what we should do if you move it (which is the same problem we have if you rename the package - should we update the name in package-config.yaml?)

for example, should we try to make these globally unique, or at least allow that, vs being relative to the repository?

relative to the repository in a deployment repo is the immediate usecase that I can relate with. Others are less clear to me as well. [Since we don't operate in context of repo in pure CLI experience, we can probably keep this no-op in CLI mode.]

About including package-type in the package paths feels like mixing logical view of your packages with the filesystem view (if they map 1:1 well, then we are good, otherwise screwed) :)

I think refactoring a package (like move package to new location or rename), I think the only way to handle them properly, will be to introduce refactoring functionality in our workflows for ex. pkg rename|move|.... if done through our tooling, we can update the underlying metadata automatically.

droot · 2022-10-14T18:20:08Z

internal/builtins/pkg_context.go

+type PackageConfig struct {
+	// PackagePath is the path to the package, as determined by the names of the parent packages.
+	// The path to a package is the parent package path joined with the package name.
+	PackagePath string


for example, should we try to make these globally unique, or at least allow that, vs being relative to the repository?

relative to the repository in a deployment repo is the immediate usecase that I can relate with. Others are less clear to me as well. [Since we don't operate in context of repo in pure CLI experience, we can probably keep this no-op in CLI mode.]

About including package-type in the package paths feels like mixing logical view of your packages with the filesystem view (if they map 1:1 well, then we are good, otherwise screwed) :)

I think refactoring a package (like move package to new location or rename), I think the only way to handle them properly, will be to introduce refactoring functionality in our workflows for ex. pkg rename|move|.... if done through our tooling, we can update the underlying metadata automatically.

droot · 2022-10-14T18:21:15Z

internal/builtins/pkg_context.go

+		if packageConfig.PackagePath != "" {
+			data[ConfigKeyPackagePath] = packageConfig.PackagePath
+		}
+	}


I am guessing this conditional will ensure package-path is not added for kpt CLI based workflows ?

I believe so, correct. It'll set it only if we have something to set.

Based on the discussion above, I sort of feel like we shouldn't be persisting either package-path or package-name into the package-context.yaml. Rather we should be generating it on the fly every time; then we don't have a conflict between directory name / directory path vs package-name/package-path.

The exception to that would be if we wanted the original name, so that if we generated something immutable it would be preserved. However, we do have other answers there, like trying to capture the name in the parameters in the kpt function pipeline or similar.

Sounds like we need to think more about the challenges here, but that we should merge this so we can gain some experience with how it behaves in practice.

droot · 2022-10-14T18:21:55Z

porch/pkg/engine/builtin.go

-		return nil, fmt.Errorf("unrecognized built-in function %q", function)
+func newPackageContextGeneratorMutation(packageConfig *builtins.PackageConfig) (mutation, error) {
+	runner := &builtins.PackageContextGenerator{
+		PackageConfig: packageConfig,


I see it now. This confirms this will be done only in porch based workflow.

droot · 2022-10-14T18:23:23Z

porch/pkg/engine/engine.go

@@ -104,10 +112,56 @@ func (cad *cadEngine) ListPackageRevisions(ctx context.Context, repositorySpec *
 	return repo.ListPackageRevisions(ctx, filter)
 }

-func (cad *cadEngine) CreatePackageRevision(ctx context.Context, repositoryObj *configapi.Repository, obj *api.PackageRevision) (repository.PackageRevision, error) {
+func buildPackageConfig(ctx context.Context, obj *api.PackageRevision, parent repository.PackageRevision) (*builtins.PackageConfig, error) {


positionalParent is how I will remember this now :)

We construct a path based on the hierarchy of parent packages. This is useful for packages/package functions that want to construct values meaningful beyond just their package - for example domain names, GCP project IDs, bucket names, descriptions etc.

justinsb requested review from droot, mortent, natasha41575 and yuwenma as code owners October 3, 2022 14:17

droot reviewed Oct 3, 2022

View reviewed changes

justinsb force-pushed the parent_path branch 2 times, most recently from 2871e2c to 5fc4084 Compare October 14, 2022 17:40

droot approved these changes Oct 14, 2022

View reviewed changes

justinsb force-pushed the parent_path branch 2 times, most recently from bfe8617 to c2d7e48 Compare October 14, 2022 20:17

justinsb force-pushed the parent_path branch from c2d7e48 to eb53a44 Compare October 14, 2022 20:22

justinsb merged commit 1f72257 into kptdev:main Oct 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate package-path metadata into package context #3596

Populate package-path metadata into package context #3596

justinsb commented Oct 3, 2022

droot left a comment

droot Oct 3, 2022

justinsb Oct 14, 2022

droot Oct 14, 2022

droot Oct 3, 2022

justinsb Oct 14, 2022

droot Oct 14, 2022

droot Oct 14, 2022

droot Oct 14, 2022

justinsb Oct 14, 2022

droot Oct 14, 2022

droot Oct 14, 2022

Populate package-path metadata into package context #3596

Populate package-path metadata into package context #3596

Conversation

justinsb commented Oct 3, 2022

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment